[No QA] add two-level gate pattern to ReportNotFoundGuard#87559
[No QA] add two-level gate pattern to ReportNotFoundGuard#87559BartekObudzinski wants to merge 2 commits intoExpensify:mainfrom
Conversation
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@DylanDylann Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 863629e452
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // - deleteTransactionNavigateBackUrl is set (always suppresses not-found), OR | ||
| // - path is valid AND report is not a transaction thread AND (still loading OR report exists) | ||
| const reportClearlyExists = !!reportID || isOptimisticDelete || userLeavingStatus; | ||
| const canSkipInnerGuard = !!deleteTransactionNavigateBackUrl || (!isInvalidReportPath && !mayBeTransactionThread && (isLoading || reportClearlyExists)); |
There was a problem hiding this comment.
Keep transaction-thread detection reactive before skipping inner guard
canSkipInnerGuard relies on mayBeTransactionThread, but isReportTransactionThread(report) depends on parent report actions that may not be in memory on first render (e.g., deep-linking directly into a transaction thread). In that case this evaluates to false, the fast path returns children, and this component no longer subscribes to parent action/metadata updates that would later reveal the thread is deleted; the Not Found view can therefore be skipped for deleted/inaccessible transaction threads until another unrelated rerender happens.
Useful? React with 👍 / 👎.
| const reportExists = !!reportID || (!isDeletedTransactionThread && isOptimisticDelete) || userLeavingStatus; | ||
|
|
||
| // eslint-disable-next-line rulesdir/no-negated-variables | ||
| const shouldShowNotFoundPage = !deleteTransactionNavigateBackUrl && (isDeletedTransactionThread || isInvalidReportPath || (!isLoading && !reportExists)); |
There was a problem hiding this comment.
This partially duplicates the outer gate, let’s clean it up.
There was a problem hiding this comment.
@BartekObudzinski This is my suggestion
outerGateCondition
const reportExists = !!reportID || isOptimisticDelete || userLeavingStatus;
const shouldShowNotFoundPage= !deleteTransactionNavigateBackUrl && (isInvalidReportPath || (!isLoading && !reportExists));
innerGateCondition
const shouldShowNotFoundPage = !deleteTransactionNavigateBackUrl && isDeletedTransactionThread
| * The inner gate mounts only when the "not found" path is plausible: | ||
| * the report is missing, the path is invalid, or the report is a transaction | ||
| * thread whose parent action may have been deleted. |
There was a problem hiding this comment.
Also, please update the comment if you agree with my suggestion
Explanation of Change
ReportNotFoundGuardpreviously subscribed to expensive Onyx keys (parentReportMetadataviauseOnyxand the full parent report actions collection viauseParentReportAction) on every render, even when the report clearly exists and the "not found" page will never be shown.This change introduces a two-level gate pattern, following the same approach already used by
LinkedActionNotFoundGuardin the codebase. The outer guard subscribes only to lightweight Onyx keys and determines whether the report clearly exists. When it does (and the report is not a transaction thread), children render directly without mounting the expensive subscriptions. The inner guard is a self-contained component that only mounts when the "not found" path is plausible: the report is missing, the path is invalid, or the report is a transaction thread whose parent action may have been deleted. The inner guard re-derives all its state from its own hooks, keeping the interface minimal (onlyreportIDFromPathandchildrenprops).Fixed Issues
$ #87632
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari